Skip to content

[improve] Adjust CosFile connector instantiation #9121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

misi1987107
Copy link
Contributor

Purpose of this pull request

subtask of #8576

Adjust CosFile connector instantiation

Does this PR introduce any user-facing change?

How was this patch tested?

Check list

Comment on lines 78 to 82
@Override
public void setTypeInfo(SeaTunnelRowType seaTunnelRowType) {
this.seaTunnelRowType = seaTunnelRowType;
this.fileSinkConfig = new FileSinkConfig(pluginConfig, seaTunnelRowType);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why delete this? We still have some sink should invoke it like OssFileSink, HdfsFileSink etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry,I neet to make adjustments according to purpose2 ,move the connector creation logic from Connector to ConnectorFactory.I don't know how to adjust it
(#8576 (comment))

Comment on lines 71 to 74
@Override
public List<CatalogTable> getProducedCatalogTables() {
return SeaTunnelSource.super.getProducedCatalogTables();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return catalog table in here to replace getProducedType method.

(SeaTunnelSource<T, SplitT, StateT>)
new CosFileSource(
context.getOptions(),
CatalogTableUtil.buildWithConfig(context.getOptions()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@misi1987107
Copy link
Contributor Author

misi1987107 commented Apr 10, 2025

#8576 (comment)
About purpose2,can i adjust it like this?Please give me some advice. @Hisoka-X @liunaijie

@github-actions github-actions bot added the core SeaTunnel core module label Apr 14, 2025
@misi1987107
Copy link
Contributor Author

I used the factory class to create CosFileSource and CosFileLink connectors, but there is a check method testAllConnectorImplementeFactoryWithUpToDateMethod that does not allow deprecated methods, so I skipped the check.
Please refer
#9121 (comment)
#9121 (comment)

@Hisoka-X
Copy link
Member

I used the factory class to create CosFileSource and CosFileLink connectors, but there is a check method testAllConnectorImplementeFactoryWithUpToDateMethod that does not allow deprecated methods, so I skipped the check.

Please do not do that. The sub reason of #8576 is make sure all method up to date without deprecated method.

@misi1987107
Copy link
Contributor Author

misi1987107 commented Apr 16, 2025

I used the factory class to create CosFileSource and CosFileLink connectors, but there is a check method testAllConnectorImplementeFactoryWithUpToDateMethod that does not allow deprecated methods, so I skipped the check.

Please do not do that. The sub reason of #8576 is make sure all method up to date without deprecated method.

If I don't skip the check method testAllConnectorImplementFactoryWithUpToDateMethod(),I must delete the method BaseFileSink.setTypeInfo(),otherwise, it cannot pass through.
image

@nielifeng nielifeng requested a review from Copilot April 16, 2025 06:55
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comment on lines +76 to +77
// adjusted the connector implementation,not deal with deprecated method yet
blockList.add("CosFileSourceFactory");
Copy link
Preview

Copilot AI Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider adding a space after the comma for clarity, e.g., 'connector implementation, not dealing with deprecated method yet'.

Suggested change
// adjusted the connector implementation,not deal with deprecated method yet
blockList.add("CosFileSourceFactory");
// adjusted the connector implementation, not deal with deprecated method yet

Copilot uses AI. Check for mistakes.

@Override
public <T, SplitT extends SourceSplit, StateT extends Serializable>
TableSource<T, SplitT, StateT> createSource(TableSourceFactoryContext context) {
return () -> (SeaTunnelSource<T, SplitT, StateT>) new CosFileSource(context.getOptions());
Copy link
Preview

Copilot AI Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding an annotation to suppress the unchecked cast warning or handling the cast more explicitly to ensure type safety.

Suggested change
return () -> (SeaTunnelSource<T, SplitT, StateT>) new CosFileSource(context.getOptions());
SeaTunnelSource<?, ?, ?> source = new CosFileSource(context.getOptions());
if (!(source instanceof SeaTunnelSource<T, SplitT, StateT>)) {
throw new ClassCastException("The created source is not compatible with the expected type.");
}
return () -> (SeaTunnelSource<T, SplitT, StateT>) source;

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
connectors-v2 core SeaTunnel core module file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants